-
Notifications
You must be signed in to change notification settings - Fork 105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add kafka importer as option #638
feat: add kafka importer as option #638
Conversation
@gasymovrv thank you for this PR - overall it looks quite good. |
src/main/java/io/zeebe/monitor/zeebe/hazelcast/ZeebeHazelcastService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gasymovrv
Ruslan, I did a local test and everything worked fine.
I made two comments. If you agree and would adjust these, then I will merge.
Again, great work 👍
docs/how-it-works.png
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, you though on updating this one.
Just being nitty gritty here, would you please adjust the arrows, following a consistent meaning (right now its mixed) - I propose 'technical dependency'.
Also, please adjust the text per each box being on the same semantical level, which makes reading easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, you're right.
My proposal was wrong at this point 🙈
docker/docker-compose.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I naively tried docker compose up
it crashed on me, because multiple backends overlap/clash.
Also, firing up too many services will stress people, who don't have enough hardware resources.
While I like your idea of these prepared services defined, may I propose to introduce profiles
.
E.g. there could be: hazelcast
, kafka
, postgres
, mysql
, and inmemory
.
Which means, we could also add a small note in the CONTRIBUTION.md
file,
saying something like: "For testing purpose, use docker profiles ... COMPOSE_PROFILES=inmemory,hazelcast docker compose up
"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Martin, it's a good idea, but I think there some troubles with multiple profiles. For example:
COMPOSE_PROFILES=in_memory,hazelcast docker compose up
will try to start all services that have "in_memory" OR "hazelcast" profile, because profiles works as "has any".
So each of these services matches:
simple-monitor-in-memory:
...
profiles: ["in_memory", "hazelcast"]
simple-monitor-in-memory-kafka:
...
profiles: ["in_memory", "kafka"]
simple-monitor-postgres:
...
profiles: ["postgres", "hazelcast"]
simple-monitor-mysql:
...
profiles: ["mysql", "hazelcast"]
The workaround is to use compound names for profiles and run docker-compose as follows: COMPOSE_PROFILES=hazelcast,hazelcast_postgres,postgres
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I see.
Thanks for pointing this out.
14380fe
to
44a41a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, Ruslan 🚀
Thank you very much for your contribution. 👍
Thank you for the good comments! I hope this feature will be useful to people |
Connected to the question:
#571